feat(sanity): display release errors #8482
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
|
No changes to documentation |
|
Component Testing Report Updated Feb 6, 2025 2:44 PM (UTC) ❌ Failed Tests (1) -- expand for details
|
⚡️ Editor Performance ReportUpdated Thu, 06 Feb 2025 14:46:34 GMT
Detailed information🏠 Reference resultThe performance result of
🧪 Experiment resultThe performance result of this branch
📚 Glossary
|
pedrobonamin
force-pushed
the
feat/content-releases
branch
from
a345b6f to
606f948
Compare
juice49
force-pushed
the
feat/corel/release-bulk-action-error-states
branch
from
bb7dacd to
d009cf3
Compare
juice49
force-pushed
the
feat/corel/release-bulk-action-error-states
branch
from
d009cf3 to
146c215
Compare
juice49
changed the title
error field
juice49
requested review from
ricokahler and
jordanl17
and removed request for
a team and
ricokahler
| description: 'undecided Error Release description', | ||
| }, | ||
| error: { | ||
| message: 'An unexpected error occurred during publication.', |
There was a problem hiding this comment.
Q: more a musing and not blocking for this work, but I wonder how we can get these sorts of error messages to work with i18n. Perhaps in the future it best we avoid just passing through the message and focus on error types so we can localise them
There was a problem hiding this comment.
I agree in principle, however error.message reflects a technical error from the server and is not intended to be presented to the user in the UI (we do output it, for debugging purposes). When we know about other types of error that will be encapsulated here, we'll need to use some heuristic to show a relevant (localised) message in the UI.
| @@ -0,0 +1,75 @@ | |||
| import {describe, expect, it} from 'vitest' | |||
|
|
|||
| import {NO_EMISSION} from '../../../../../test/matchers/toMatchEmissions' | |||
| switchMap(({releases}) => | ||
| from(releases.values()).pipe( | ||
| filter((release) => release.state === 'active'), | ||
| filter((release) => typeof release.error !== 'undefined'), |
There was a problem hiding this comment.
COULD: roll this as a single filter, then since the check on state active and typeof error in used elsewhere too, could abstract that as some util... thinking maybe if the api simplifies or changes in the future that makes it easier
There was a problem hiding this comment.
Agree it would make sense to abstract this to some guards:
function isActiveRelease(maybeActiveRelease): maybeActiveRelease is ReleaseDocument & { state: 'active' }function isErrorRelease(maybeErrorRelease): maybeErrorRelease is ReleaseDocument & { error: { message: string } }These could then be composed to abstract the logic around whether an error should be displayed.
| id: 'error', | ||
| sorting: false, | ||
| width: 40, | ||
| header: () => <Fragment />, | ||
| cell: ({datum: {error}, cellProps}) => ( | ||
| <Flex | ||
| {...cellProps} | ||
| align="center" | ||
| paddingX={2} | ||
| paddingY={3} | ||
| sizing="border" | ||
| data-testid="error-indicator" | ||
| > | ||
| {typeof error !== 'undefined' && ( | ||
| <Tooltip content={<Text size={1}>{t('failed-publish-title')}</Text>} portal> | ||
| <Text size={1}> | ||
| <ToneIcon icon={ErrorOutlineIcon} tone="critical" /> | ||
| </Text> | ||
| </Tooltip> | ||
| )} | ||
| </Flex> | ||
| ), | ||
| }, |
There was a problem hiding this comment.
SHOULD: we only want this to show for active releases, right? Either could return a different list of cols for active and archived; or perhaps could just add in the datum.state === 'active' check here too
There was a problem hiding this comment.
Very good point, thank you! I missed this detail 🤦♀️.
There was a problem hiding this comment.
I've updated the code to simply perform a check on the cell datum, as you suggested. We should abstract this in the future, but I decided not to for now, as we're about to learn more about the errors that can be stored and our heuristics will possibly change.
juice49
force-pushed
the
feat/test-rxjs-emissions
branch
from
7c02363 to
c135ab5
Compare
juice49
force-pushed
the
feat/corel/release-bulk-action-error-states
branch
from
279a369 to
05cf15c
Compare
Description
This branch adds release errors to the data model and updates the UI to reflect the presence of a release error. Presence of an error is only taken into account for releases that are in an active state.
This branch does not include a way to deal with a release error. We'll likely want to allow folks to manually publish release, allowing the remaining unpublished documents to be published. Decided to exclude this work for now, because a lot of the publication logic is encoded into the publish button itself, making it tricky to extract into a different context. This is expected to be a very narrow edge case.
What to review
When there are errors present
The releases tool icon is given a small error indicator
The releases list view will display an error indicator in the affected row
The release detail view will show an error indicator at the start
The release detail view will show an error detail UI
Testing
Notes for release
Adds release error details to UI.